Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@/CivicSignalBlog: Add Web Tools authentication form configuration #1009

Merged
merged 20 commits into from
Dec 10, 2024

Conversation

m453h
Copy link
Contributor

@m453h m453h commented Nov 28, 2024

Description

This PR adds configuration globals for authentication forms used in the CivicSignal CMS.

Partially fixes this issue opened in the CivicSignal web-tools

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Screenshot 2024-11-28 at 09 08 16

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@m453h m453h added the enhancement New feature or request label Nov 28, 2024
@m453h m453h self-assigned this Nov 28, 2024
@kilemensi
Copy link
Member

Looks good @m453h (haven't checked the code yet). A few quick thoughts (totally open for discussion)

  1. Why not have Forms as a separate group (rather than settings) where we can have Login form, Registration form, etc.
  2. Instead of Input Elements and Form Messages as separate groups, why not combine them into one group Fields? Then for each field, you'll have two entries: label and errorMessage e.g. email.label, and email.errorMessage. Additionally there can be a third entry helpMessage (or placeholder) if a field needs to be extra descriptive.

Of course, we may need a common place to put those common message that are the same across all forms, etc., but may be we don't need it now? If we do, then this can go into Settings.

@m453h
Copy link
Contributor Author

m453h commented Nov 28, 2024

  • Why not have Forms as a separate group (rather than settings) where we can have Login form, Registration form, etc.
    Hmmm 🤔 I like the idea of splitting the forms... let me move the components as per this suggestion and group the inputs as well, I think it would greatly improve the user experience

@m453h
Copy link
Contributor Author

m453h commented Dec 2, 2024

  • Why not have Forms as a separate group (rather than settings) where we can have Login form, Registration form, etc.

Hmmm 🤔 I like the idea of splitting the forms... let me move the components as per this suggestion and group the inputs as well, I think it would greatly improve the user experience

@m453h
Copy link
Contributor Author

m453h commented Dec 2, 2024

  • Why not have Forms as a separate group (rather than settings) where we can have Login form, Registration form, etc.

Hmmm 🤔 I like the idea of splitting the forms... let me move the components as per this suggestion and group the inputs as well, I think it would greatly improve the user experience

@kilemensi, let me know if this arrangement for the login form makes more sense

Screen.Recording.2024-12-02.at.16.05.26.mov

@m453h m453h requested a review from kilemensi December 2, 2024 13:09
Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽 ... Definitely on the right track @m453h .


Not sure about "dynamic" fields. Don't we need to match names to what APIs expect e.g. email field must be called email, etc. If content editor renames it to emailAddress, will the form still work?

Besides that:
Screenshot 2024-12-02 at 17 10 30
Lets arrange the sections as Publication, Forms and then Settings last.

Screenshot 2024-12-02 at 17 10 55

Lets arrange forms as Title/Description, Fields, Actions (instead of Buttons as some actions could just be/look like links), and Messages (instead of Form Messages) last.

@m453h
Copy link
Contributor Author

m453h commented Dec 2, 2024

👍🏽 ... Definitely on the right track @m453h .

Not sure about "dynamic" fields. Don't we need to match names to what APIs expect e.g. email field must be called email, etc. If content editor renames it to emailAddress, will the form still work?

Yes we need to match the names, I had conflicting thoughts about dynamic fields (I opted to use them since they reduce repetitions in the code) and had kept a validator to ensure all the expected names from the front-end are included, since I have a function that creates these fields, I can quickly repurpose it to have pre-defined fields and remove this validator

Copy link
Contributor

@koechkevin koechkevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m453h m453h requested a review from koechkevin December 5, 2024 10:51
@m453h
Copy link
Contributor Author

m453h commented Dec 10, 2024

@kilemensi to add a bit of context, I am using TextField in the Login, PasswordReset and Registration Forms instead of E-mail because, within the CMS, we expect the user to set what would appear as labels, error messages and hints on the font-end forms thus the user would always need to enter free text, using the E-mail field wouldn't work in this scenario.

@kilemensi
Copy link
Member

Cool, cool @m453h ... got you now. The forms here are just for collecting messages (error, hints, etc.)

@kilemensi
Copy link
Member

Don't wait for me on this PR @m453h but did you fix the titles e.g. Form Messages to just messages, Buttons to Actions, etc.

Screenshot 2024-12-10 at 14 02 22 Screenshot 2024-12-10 at 14 02 58

@m453h
Copy link
Contributor Author

m453h commented Dec 10, 2024

Don't wait for me on this PR @m453h but did you fix the titles e.g. Form Messages to just messages, Buttons to Actions, etc.

Screenshot 2024-12-10 at 14 02 22 Screenshot 2024-12-10 at 14 02 58

Sure, just cross-checked, I had one commit I didn't push for the labels...I think everything else should be alright

@m453h m453h added this pull request to the merge queue Dec 10, 2024
Merged via the queue into main with commit b140c7f Dec 10, 2024
4 checks passed
@m453h m453h deleted the ft/civicsignalblog-auth_forms branch December 10, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

@/CivicSignal UI - Fix password reset Form
3 participants